API changes and batch schedule#12796
Conversation
JoshLove-msft
requested review from
KrzysztofCwalina,
MiYanni,
ShivangiReja,
erich-wang,
isra-fel,
jsquire,
pakrym and
tg-msft
as code owners
JoshLove-msft
force-pushed
the
api-change-schedule
branch
from
270e12a to
6137fda
Compare
| /// | ||
| /// <returns>List of messages received. Returns an empty list if no message is found.</returns> | ||
| public override async Task<IList<ServiceBusReceivedMessage>> ReceiveBatchAsync( | ||
| public override async Task<IList<ServiceBusReceivedMessage>> ReceiveMessagesAsync( |
There was a problem hiding this comment.
I like this change; It feels more natural and reads better to me.
| /// <param name="cancellationToken">An optional <see cref="CancellationToken"/> instance to signal the request to cancel the operation.</param> | ||
| /// | ||
| /// <returns>The batch of <see cref="ServiceBusMessage" /> from the Service Bus entity this receiver is associated with. If no messages are present, an empty list is returned.</returns> | ||
| /// <returns>The list of <see cref="ServiceBusMessage" /> from the Service Bus entity this receiver is associated with. If no messages are present, an empty list is returned.</returns> |
There was a problem hiding this comment.
Maybe consider "set" as a more neutral term? "List" makes me think concrete implementation, which isn't wrong here, given the IList return type, but the "list" form isn't really important in the context.
There was a problem hiding this comment.
I went back and forth on this but since it returns an IList, list feels appropriate. "Set" might also mean a concrete implementation to some. Alternatively, I can just cref the whole IList but that felt unnecesary.
There was a problem hiding this comment.
I'm good whichever way you feel is best.
There was a problem hiding this comment.
This might be something to look at in terms of consistency across the Track 2 libraries, as this surely isn't a new issue. Though HTTP libraries use AsyncPageable so the nomenclature would be different. I guess we should at least try to align across SB/EH.
| /// </summary> | ||
| /// | ||
| public class CreateBatchOptions | ||
| public class CreateMessageBatchOptions |
There was a problem hiding this comment.
I'm not sure that this rename offers much. Were we concerned that there would be confusion with another set of batch creation options?
I wouldn't think that this type is something that developers would need to discover from a list on its own, but would instead be guided by the parameter type/name when explicitly calling to create a batch. At that point, I think the purpose is clear.
There was a problem hiding this comment.
Well, we renamed the operation to CreateMessageBatchAsync, so this maintains consistency with that. Do you have the same concerns with renaming to CreateMessageBatchAsync?
There was a problem hiding this comment.
I do. I'm bucketing them both under the question "does adding more words add more clarity or just give readers more things to parse?" That said, I don't have particularly strong feelings.
There was a problem hiding this comment.
I think it adds more clarity and also consistency. We are now adding Message/Messages for all service operations that operate on a message. It would be strange to not do the same here.
4 participants
Uh oh!
There was an error while loading. Please reload this page.